Changed TextField label font size to prevent overflowing parent #108
Conversation
Justin Tyler Campbell (campb303)
left a comment
Justin Tyler Campbell (campb303)
left a comment
There was a problem hiding this comment.
We cannot arbitrarily change font sizes, especially not as a fix for text appearance. Rather than making the text smaller, we should look for CSS solutions that allow for arbitrary text to be entered and appear properly. Consider using state and multiple props to display the text when the filter field is and is not active.
That aside, be sure to remove unnecessary spacing and unused imports. Whitespace doesn't affect functionality but it has significance for readability and should be treated with care. Unused imports throw eslint errors and create performance issues but loading and compiling code that is never used.
We'll need to address the comments before we can merge this change in.
| @@ -1,19 +1,35 @@ | |||
| import React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import { TextField } from "@material-ui/core"; | |||
| import { TextField, makeStyles } from "@material-ui/core"; | |||
| import { pink, purple } from '@material-ui/core/colors'; | |||
There was a problem hiding this comment.
Are these colors needed? They've been imported but aren't used.
|
|
||
| export default function ItemTableFilter({ label, onChange }) { | ||
|
|
||
| const useStyles = makeStyles({ | ||
|
|
There was a problem hiding this comment.
Remove unnecessary spacing.
| fontSize: 14, | ||
| }, | ||
|
|
||
| }, |
There was a problem hiding this comment.
Combine these block closers )};
| ); | ||
|
|
||
| const classes = useStyles(); | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
React fragments <> and </> are only needed if there is more than one root element being returned from a component. Because you're only using the TextField component here, you don't need fragments. Please remove them.
| labelRoot: { | ||
| fontSize: 14, | ||
| }, | ||
|
|
There was a problem hiding this comment.
Remove unnecessary spacing.
| color="secondary" | ||
| type="search" | ||
| size="small" | ||
| variant="outlined" | ||
| fullWidth | ||
|
|
There was a problem hiding this comment.
Remove unnecessary spacing.
| <TextField | ||
| label={label} | ||
| onChange={onChange} | ||
| classes={{ root: classes.inputLabelStyles }} | ||
| InputLabelProps={{ | ||
| classes: { root: classes.labelRoot, focused: classes.labelFocused }}} |
There was a problem hiding this comment.
classes.labelFocused doesn't exist. Should it be created in useStyles() or removed from this classes prop?
| const useStyles = makeStyles({ | ||
|
|
||
| labelRoot: { | ||
| fontSize: 14, |
There was a problem hiding this comment.
We can't arbitrarily change font sizes. Its a nightmare for accessibility and constancy. We'll need a solution that scales with whatever content we put into the filter rows.
|
This is related to #105 |
Justin Tyler Campbell (campb303)
left a comment
Justin Tyler Campbell (campb303)
left a comment
There was a problem hiding this comment.
Be sure to remove unused imports.
| @@ -1,37 +1,33 @@ | |||
| import React from 'react'; | |||
| import React, { useEffect, useState } from 'react'; | |||
There was a problem hiding this comment.
useEffect is not used in this component and should be removed.
Justin Tyler Campbell (campb303)
deleted the
bugfix-ItemTableFilter-label-overflow
branch
2 participants
No description provided.